Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Ellipsoid surface area #8986

Merged
merged 6 commits into from
Jun 24, 2020
Merged

Added Ellipsoid surface area #8986

merged 6 commits into from
Jun 24, 2020

Conversation

ErixenCruz
Copy link
Contributor

Fixed Ellipsoid.geodeticSurfaceNormal dividing by zero when given origin as input.
Added Ellipsoid.surfaceArea for computing the surface area under given rectangle.

@ErixenCruz ErixenCruz requested a review from lilleyse June 23, 2020 15:50
@cesium-concierge
Copy link

Thank you so much for the pull request @ErixenCruz! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I was reading this again just noticed a few areas where the wording could be clearer. Otherwise looks good!

CHANGES.md Outdated
- Added support for PolylineVolume in CZML [#8841](https://github.com/CesiumGS/cesium/pull/8841)

##### Fixes :wrench:

- Fixed `Ellipsoid.geodeticSurfaceNormal` dividing by zero when given origin as input.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Fixed `Ellipsoid.geodeticSurfaceNormal` dividing by zero when given origin as input.
- Fixed a divide-by-zero bug in `Ellipsoid.geodeticSurfaceNormal` when given the origin as input. `undefined` is returned instead. [#8986](https://github.com/CesiumGS/cesium/pull/8986)

CHANGES.md Outdated
@@ -8,10 +8,12 @@

##### Additions :tada:

- Added `Ellipsoid.surfaceArea` for computing the surface area under given rectangle.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Added `Ellipsoid.surfaceArea` for computing the surface area under given rectangle.
- Added `Ellipsoid.surfaceArea` for computing the approximate surface area of a rectangle on the surface of an ellipsoid.

@@ -3,6 +3,7 @@ import { Cartographic } from "../../Source/Cesium.js";
import { Ellipsoid } from "../../Source/Cesium.js";
import { Math as CesiumMath } from "../../Source/Cesium.js";
import createPackableSpecs from "../createPackableSpecs.js";
import { Rectangle } from "../../Source/Cesium.js";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to keep things tidy, move this import above one line so it's next to the other Cesium.js imports.

*/

/**
* Computes an approximation of the surface area of a given section of the surface of an ellipsoid using
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Computes an approximation of the surface area of a given section of the surface of an ellipsoid using
* Computes an approximation of the surface area of a rectangle on the surface of an ellipsoid using

* Gauss-Legendre 10th order quadrature.
*
* @param {Rectangle} rectangle The rectangle to find the surface area of.
* @returns {Number} The approximate area of the section on the surface of this ellipsoid.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @returns {Number} The approximate area of the section on the surface of this ellipsoid.
* @returns {Number} The approximate area of the rectangle on the surface of this ellipsoid.

* Computes an approximation of the surface area of a given section of the surface of an ellipsoid using
* Gauss-Legendre 10th order quadrature.
*
* @param {Rectangle} rectangle The rectangle to find the surface area of.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {Rectangle} rectangle The rectangle to find the surface area of.
* @param {Rectangle} rectangle The rectangle used for computing the surface area.

@ErixenCruz
Copy link
Contributor Author

Ok. That last commit should do it @lilleyse Thanks for the feedback.

@ErixenCruz
Copy link
Contributor Author

Just kidding I missed the CHANGES.md changes but that last commit should do it.

@ErixenCruz ErixenCruz force-pushed the ellipsoid-surfacearea branch from 1f6e9cd to 5255bb6 Compare June 24, 2020 13:46
@ErixenCruz
Copy link
Contributor Author

I know I'm not supposed to force, but I did it to fix typo in commit message.

@lilleyse
Copy link
Contributor

Thanks @ErixenCruz!

@lilleyse lilleyse merged commit 6f4fadf into master Jun 24, 2020
@lilleyse lilleyse deleted the ellipsoid-surfacearea branch June 24, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants